Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(storage/linstor): Enhance multi-disk support and provisioning flexibility in Linstor SR tests #270

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rushikeshjadhav
Copy link
Contributor

@rushikeshjadhav rushikeshjadhav commented Feb 5, 2025

  • Added sr_disks_for_all_hosts fixture to support multiple disks, ensuring availability across all hosts and handling "auto" selection dynamically.
  • Updated lvm_disk fixture to provision multiple disks collectively, refining vgcreate and pvcreate logic.
  • Introduced provisioning_type and storage_pool_name fixtures to dynamically configure storage provisioning (thin or thick).
  • Refactored Linstor SR test cases to use the new fixtures, improving test coverage across provisioning types.
  • Optimized Linstor installation and cleanup using concurrent execution, reducing setup time.
  • Enhanced validation and logging for disk selection.

@stormi
Copy link
Member

stormi commented Feb 6, 2025

Adding a new CLI parameter such as --provisioning is a last resort change, because it moves the responsibility for defining it to the caller, when we probably want that our tests, by default, provide both testing for thick and thin provisioning, as their definition.

Couldn't we have test definitions that do both thick and thin, instead?

@rushikeshjadhav
Copy link
Contributor Author

Couldn't we have test definitions that do both thick and thin, instead?

We can do that. Since this is XOSTOR level, one full cycle (create-test-destroy) with thin and then another full cycle with thick can be added.

Can we retain the CLI option for those callers who want to selectively run in one of the mode?

@stormi
Copy link
Member

stormi commented Feb 10, 2025

Can we retain the CLI option for those callers who want to selectively run in one of the mode?

In theory pytest already offers enough selectors to execute a set of tests and not others.

@rushikeshjadhav rushikeshjadhav changed the title feat(storage/linstor): Add provisioning type support and multi-disk enhancements for Linstor SR tests feat(storage/linstor): Enhance multi-disk support and provisioning flexibility in Linstor SR tests Feb 12, 2025
@rushikeshjadhav rushikeshjadhav force-pushed the feat-storage-linstor branch 3 times, most recently from 989afea to 5b0e1b7 Compare February 12, 2025 13:47
@rushikeshjadhav rushikeshjadhav marked this pull request as ready for review February 12, 2025 13:47
Comment on lines 162 to 163
"thick": "xcp-sr-linstor_group_device",
"thin": "xcp-sr-linstor_group_thin_device"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use conftest.py here, we already have these helpers:

GROUP_NAME = 'linstor_group'
STORAGE_POOL_NAME = f'{GROUP_NAME}/thin_device'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also xcp-sr-linstor_group_device is not valid, it's just xcp-sr-linstor_group_device for thick provisioning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, using GROUP_NAME is better.

'redundancy': '1',
'provisioning': 'thin'
'group-name': storage_pool_name,
'redundancy': '2',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here.

'redundancy': '1',
'provisioning': 'thin'
'group-name': storage_pool_name,
'redundancy': '2',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason to set 2 in your tests?

Copy link
Contributor Author

@rushikeshjadhav rushikeshjadhav Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a strange issue with thick SR and redundancy set as 1. The error looks like below upon trying to create a VM (VDI). So, keeping it as 2 for now, so that other tests on thick SR can pass.

***** LINSTOR resources on XCP-ng: EXCEPTION <class 'xs_errors.SROSError'>, VDI Creation failed [opterr=error cmd: `['/usr/bin/vhd-util', 'create', '--debug', '-n', '/dev/drbd/by-res/xcp-volume-d21c6a82-ed29-4106-ace6-7542c334254c/0', '-s', '2048', '-S', '2097152']`, code: `30`, reason: `/dev/drbd/by-res/xcp-volume-d21c6a82-ed29-4106-ace6-7542c334254c/0: failed to create: -30` (openers: {'xcp-ng-xs1': {}})]
Feb 18 19:02:59 xcp-ng-xs1 SM: [46577]   File "/opt/xensource/sm/LinstorSR", line 1743, in create

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we must handle this error in the driver itself. In theory a simple busy loop should fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will keep it as 1.

conftest.py Outdated
assert len(disks) > 0, "This test requires at least one --sr-disk parameter"
# Fetch available disks on the master host
master_disks = host.available_disks()
assert len(master_disks) > 0, "a free disk device is required on the master host"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe A instead of a to uniformize.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

@@ -12,57 +12,85 @@
LINSTOR_PACKAGE = 'xcp-ng-linstor'

@pytest.fixture(scope='package')
def lvm_disk(host, sr_disk_for_all_hosts):
device = '/dev/' + sr_disk_for_all_hosts
def lvm_disk(host, sr_disks_for_all_hosts, provisioning_type):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe lvm_disks if you change the logic here. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes :)

tests/storage/linstor/conftest.py Show resolved Hide resolved
…exibility in Linstor SR tests

- Added `sr_disks_for_all_hosts` fixture to support multiple disks, ensuring availability across all hosts and handling "auto" selection dynamically.
- Updated `lvm_disks` (`lvm_disk`) fixture to provision multiple disks collectively, refining vgcreate and pvcreate logic.
- Introduced `provisioning_type` and `storage_pool_name` fixtures to dynamically configure storage provisioning (thin or thick).
- Refactored Linstor SR test cases to use the new fixtures, improving test coverage across provisioning types.
- Optimized Linstor installation and cleanup using concurrent execution, reducing setup time.
- Enhanced validation and logging for disk selection.

Signed-off-by: Rushikesh Jadhav <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants